Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reworked TMHM into expandable list format #2233

Merged
merged 10 commits into from
Aug 29, 2022

Conversation

gruxor
Copy link

@gruxor gruxor commented Aug 24, 2022

Description

Completely removed the need for bit arrays, instead using the format of gLevelUpLearnsets as a guide to rework TMs/HMs entirely. TMHM lists use MOVE_UNAVAILABLE as a footer to allow proper looping, since dynamic arrays can't have their sizes determined at compile time. gLevelUpLearnsets uses an identical method through LEVEL_UP_END which shares the same value (max size u16, i.e. 0xFFFF). Optionally, a new preprocessor define could be used instead for the footer to prevent issues with any future integer type modifications.

  • Should be much friendlier for new faces and less of a headache for old faces, with virtually no difference in performance/memory that I'm aware of.
  • Should cover all use cases - functions that the data was/is fed into have all been accounted for across the entire project (there weren't many)
  • Haven't yet touched the tutor movelist (which uses an almost identical bit array system) but I imagine someone wanting to expand that list is much more rare. Could be easily done in the future if necessary, or just for consistency/ease of use
  • Movelists may have minor errors since I regex'd the old list during conversion, but I'm pretty sure the compiler already caught most if not all of those during the testing process
  • Support for PoryMoves should be a fairly trivial modification since the app already supports leveled lists
  • Probably conflicts with Implemented support for 128 bit tmhm learnsets. #2232 (sorry Stephen) as it's an alternative solution to the same problem, but without the 128-bit limitations and with added user-friendliness

Discord contact info

Crater#7777

@StephenLynx
Copy link

You skipped the use of P_NEW_POKEMON to enable some entries?

@gruxor
Copy link
Author

gruxor commented Aug 24, 2022

You skipped the use of P_NEW_POKEMON to enable some entries?

Checks for P_NEW_POKEMON still exist in tmhm_learnset_pointers.h, which determines if the arrays are actually used. They're statically defined then added to gTMHMLearnsets and can't really be used without it (and it'd defeat the purpose to access them directly anyways).

Preprocessor if statements could be introduced (re-introduced?) to tmhm_learnets.h, but I doubt it really makes much of a difference in the end.

edit: just checked, only 2 exist in level_up_learnsets.h, so moving those over would be super trivial if they're wanted for reasons that I'm missing or just for consistency's sake

@AsparagusEduardo
Copy link
Collaborator

P_NEW_POKEMON is meant to save space, and therefore, any data related to the new mon shouldn't be compiled into the rom if it's not gonna be used.

@gruxor
Copy link
Author

gruxor commented Aug 24, 2022

Fixed.

@StephenLynx
Copy link

Speaking of space, I think it's important to keep in mind this will at least double the space used for this, maybe even triple.

@gruxor
Copy link
Author

gruxor commented Aug 24, 2022

The space taken up is pretty negligible. It's certainly more than the existing table and even the 128-bit expansion, but it's still incredibly tiny.

The current master branch of the expansion, even with nothing disabled (all the cries, graphics, etc.) and with this change added, takes up only about 23.8MB of the available 32MB in the ROM.

Copy link
Collaborator

@AsparagusEduardo AsparagusEduardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 additional things:

  • Spaces instead of tabs in tmhm_learnsets.h

  • Something that I've been thinking is that in vanilla, TMs/HMs and Tutor Moves don't overlap in terms of moves that can be taught. And even if they did, I doubt that they would be able to be learned with one method and not the other. My thought is that we could potentially unify both TM and Tutor tables into a single list, seeing as not only CanMonLearnTMTutor is already used by both, but it would further simplify modifying both Tutors and TMs (eg: no need for TUTOR_MOVE_* and stuff).

src/daycare.c Outdated Show resolved Hide resolved
src/pokemon.c Outdated Show resolved Hide resolved
Co-authored-by: Eduardo Quezada D'Ottone <[email protected]>
@gruxor
Copy link
Author

gruxor commented Aug 25, 2022

Updated the tabs to spaces, haven't committed it yet pending the following:

Something that I've been thinking is that in vanilla, TMs/HMs and Tutor Moves don't overlap in terms of moves that can be taught. And even if they did, I doubt that they would be able to be learned with one method and not the other. My thought is that we could potentially unify both TM and Tutor tables into a single list, seeing as not only CanMonLearnTMTutor is already used by both, but it would further simplify modifying both Tutors and TMs (eg: no need for TUTOR_MOVE_* and stuff).

I can see that working for most use cases, but not all. For example, if you have one-use TMs (or TRs, if those are implemented) then you could give the player one for a fairly powerful move early on. Then later, maybe you want to grant access to a tutor that can teach it infinitely in exchange for a Star Piece or something. Can't imagine stuff like that would be super common, but it isn't out of the question.

I think it'd definitely still be possible to trim down some of the code if I set up the tutor movelists in the same way, though. I can give it a shot if it'd be better to merge it alongside this change rather than via a separate PR.

@AsparagusEduardo
Copy link
Collaborator

I can see that working for most use cases, but not all. For example, if you have one-use TMs (or TRs, if those are implemented) then you could give the player one for a fairly powerful move early on. Then later, maybe you want to grant access to a tutor that can teach it infinitely in exchange for a Star Piece or something. Can't imagine stuff like that would be super common, but it isn't out of the question.

I don't how that couldn't work with what I proposed. If the mon is able to learn the move with a single-use TM, it should also be able to learn that move from that Star Piece Tutor if both instances are read from the same move compatibility table.

@gruxor
Copy link
Author

gruxor commented Aug 25, 2022

I don't how that couldn't work with what I proposed. If the mon is able to learn the move with a single-use TM, it should also be able to learn that move from that Star Piece Tutor if both instances are read from the same move compatibility table.

That's true, I don't know why I was thinking the method would matter. The thing in question would be "should a species be able to learn a certain move from a TM and not a Tutor, or vice versa?" and I really can't think of any examples where anyone would want that.

I'll see what I can do, but I'll probably have to set up a script of some sort this time since I'll need to merge the arrays.

@AsparagusEduardo
Copy link
Collaborator

I'll see what I can do, but I'll probably have to set up a script of some sort this time since I'll need to merge the arrays.

Either that, or we could wait until I add beta PoryMoves support for this 👀. Don't know how long that would take though, I tend to get distracted a lot lol

@gruxor
Copy link
Author

gruxor commented Aug 26, 2022

Massive changes with ed02c4b. Boom.

  • Merged tutor and TMHM learnlists into one
  • Removed incredibly stupid arg requirements for the "can this pokemon learn this move" functions - now take in a move ID instead of needing to pass through a dummy item ID or dummy tutor ID based on the usage
  • Removed gTutorMoves entirely, tutors now use move IDs directly rather than an abstracted list
  • Removed more than a handful redundant carry-overs from the leveled lists. Lists existed for forms that have no differences, such as the Vivillion colors and many others. These were already not being used, because the pointer lists never assigned anything to them, instead using their base form. I'll probably submit a separate PR to clean up the leveled lists in the same way since that's the source of the redundant lists.

I did testing with Treecko to check TMs, HMs, and tutors as well as move-overwriting for each. May need additional testing though. Initially had issues regarding the move being stored in gPartyMenu.data1, but I'm 99% sure I fixed all quirks and bugs on that front already.

src/pokemon.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@AsparagusEduardo AsparagusEduardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, this is great to see!

Though, there are some things that I feel should be changed name-wise:

  • gTeachLearnsets ->gTMTutorLearnsets

  • CanMonLearnTaughtMove -> CanMonLearnTMTutorMove

  • CanSpeciesLearnTaughtMove -> CanSpeciesLearnTMTutorMove

  • CanMonLearnMove -> CanMonLearnTMTutor

  • teach_learnset_pointers.h ->tm_tutor_learnset_pointers.h

  • teach_learnsets.h -> tm_tutor_learnsets.h

  • sSPECIESTeachLearnset -> sSPECIESTMTutorLearnset

src/field_specials.c Outdated Show resolved Hide resolved
@gruxor
Copy link
Author

gruxor commented Aug 26, 2022

... Though, there are some things that I feel should be changed name-wise: ...

Discussed on Discord, updated in f8fb4e8.

@gruxor
Copy link
Author

gruxor commented Aug 28, 2022

Rushed the initial changes to Battle Frontier and assumed some variables meant something they didn't actually. Now, having fully deciphered the spaghetti, the tutors should actually be fixed in 9f16d1d.

Gotta love multi-use unnamed variables.

src/apprentice.c Outdated Show resolved Hide resolved
@AsparagusEduardo AsparagusEduardo dismissed DizzyEggg’s stale review August 29, 2022 08:20

Requested changes were implemented.

@AsparagusEduardo AsparagusEduardo merged commit 8ca2f94 into rh-hideout:master Aug 29, 2022
Greenphx9 added a commit to Greenphx9/pokefirered that referenced this pull request Sep 17, 2022
@gruxor gruxor deleted the tmhm_rework branch September 24, 2022 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants